Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(protocol-designer): analytics opt in modal fixes #17106

Open
wants to merge 9 commits into
base: chore_release-pd-8.2.1
Choose a base branch
from

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Dec 13, 2024

closes AUTH-1142

Overview

Fix when the opt in modal shows up and also adjust the copy and CTAs.

Test Plan and Hands on Testing

Test that this works in your dev env with the gate modal ff turned on like so: make -C protocol-designer dev OT_PD_SHOW_GATE=true. Test that if you select to accept and then go to the settings to toggle it off, it doesn't show up again unless you clear local cache.

Changelog

update modal to match designs

code wise, i added an appVersion prop to the opt_in action. If the appVersion is undefined, it will trigger the modal. also if the opt_in is null, it will trigger the modal. i believe this should force the user to see the modal even if they previously selected false.

Risk assessment

low

@jerader jerader requested a review from a team as a code owner December 13, 2024 18:08
@jerader jerader requested review from koji and ncdiehl11 December 13, 2024 18:10
@jerader jerader requested a review from shlokamin December 13, 2024 18:10
@jerader jerader changed the base branch from edge to chore_release-pd-8.2.1 December 13, 2024 18:10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not directly related to this pr but seems that dispatch in trackEventMiddleware isn't used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll leave that to a follow up unless you think its affecting the modal?

@@ -499,7 +499,7 @@ export const trackEventMiddleware: Middleware<BaseState, any> = ({
// NOTE: this is the Redux state AFTER the action has been fully dispatched
const state = getState()

const optedIn = getHasOptedIn(state as BaseState) ?? false
const optedIn = getHasOptedIn(state as BaseState)?.hasOptedIn ?? false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not 100% sure but the request is mixpanel is on as the default setting so this might be true?

@jerader jerader marked this pull request as draft December 13, 2024 19:09
@jerader
Copy link
Collaborator Author

jerader commented Dec 13, 2024

changed this back to a draft because the modal isn't working as I thought it was

@koji
Copy link
Contributor

koji commented Dec 13, 2024

changed this back to a draft because the modal isn't working as I thought it was

i guess that might be dispatch part that I pointed out.

@jerader jerader marked this pull request as ready for review December 13, 2024 19:44
Comment on lines -62 to -64
const showGateModal =
process.env.NODE_ENV === 'production' || process.env.OT_PD_SHOW_GATE

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it cool with yall if i remove this? I don't think it'll get too annoying throughout pd development and it didn't seem to be working correctly in production (i tested my PD in prod and even with optIn as undefined, the modal still didn't show up) What do you think @shlokamin @koji @ncdiehl11 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants